Skip to content

Switch to iterative algorithm to prevent stack overflows. #40211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Dec 13, 2019
Merged

Switch to iterative algorithm to prevent stack overflows. #40211

merged 9 commits into from
Dec 13, 2019

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Dec 6, 2019

Fixes #40041

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 6, 2019 20:04
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 6, 2019

Tagging @sharwell

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Dec 6, 2019
Closed
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 6, 2019

Note: i'd recommend reviewing this a commit at a time.

CyrusNajmabadi
CyrusNajmabadi commented Dec 6, 2019
View reviewed changes
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i preserved this logic (just in O(1) time instead of O(n)). But, overall, i think preserving this logic is pretty unnecessary. i don't see why we actually need it.

CyrusNajmabadi
CyrusNajmabadi commented Dec 6, 2019
View reviewed changes
src/EditorFeatures/CSharp/Highlighting/KeywordHighlighters/AsyncAwaitHighlighter.cs
spans[spans.Count - 1] = TextSpan.FromBounds(lastSpan.Start, mod.Span.End);
return true;
}
}
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i preserved this logic (just in O(1) time instead of O(n)). But, overall, i think preserving this logic is pretty unnecessary. i don't see why we actually need it.

Copy link
Contributor

@AdamSpeight2008 AdamSpeight2008 Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It highlights the entire async code section rather than the highlighting the nested inside keywords Async and Await.
Personally i prefer the latter.

CyrusNajmabadi
CyrusNajmabadi commented Dec 6, 2019
View reviewed changes
src/EditorFeatures/CSharp/Highlighting/KeywordHighlighters/AsyncAwaitHighlighter.cs Outdated
return;
case LocalDeclarationStatementSyntax localDeclaration:
if (localDeclaration.AwaitKeyword.Kind() == SyntaxKind.AwaitKeyword && localDeclaration.UsingKeyword.Kind() == SyntaxKind.UsingKeyword)
if (localDeclaration.UsingKeyword.Kind() == SyntaxKind.UsingKeyword)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i preserved this logic. But i dont' actually think it's necessary. this basically ensures that we only highlight await in await using var whatever and not in await var whatever. However, the latter is broken code, so it would be fine IMO to highlight there.

CyrusNajmabadi
CyrusNajmabadi commented Dec 6, 2019
View reviewed changes
src/EditorFeatures/CSharp/Highlighting/KeywordHighlighters/AsyncAwaitHighlighter.cs
AnonymousFunctionExpressionSyntax anonymousFunction => TryAddAsyncOrAwaitKeyword(anonymousFunction.AsyncKeyword, spans),
UsingStatementSyntax usingStatement => TryAddAsyncOrAwaitKeyword(usingStatement.AwaitKeyword, spans),
LocalDeclarationStatementSyntax localDeclaration =>
localDeclaration.UsingKeyword.Kind() == SyntaxKind.UsingKeyword && TryAddAsyncOrAwaitKeyword(localDeclaration.AwaitKeyword, spans),
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i preserved this logic. But i dont' actually think it's necessary. this basically ensures that we only highlight await in await using var whatever and not in await var whatever. However, the latter is broken code, so it would be fine IMO to highlight there.

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Dec 6, 2019
@AdamSpeight2008
Copy link
Contributor

AdamSpeight2008 commented Dec 9, 2019

The would also need applying to the vb side as well.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 9, 2019

@AdamSpeight2008 my goal is not to harden the entirely of the IDE codebase to stack overflows.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 9, 2019

@sharwell LMK what you need here to proceed.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 12, 2019

@sharwell @jinujoseph LMK what you need here to proceed.

@sharwell
Copy link
Contributor

sharwell commented Dec 13, 2019 •
edited
Loading

@CyrusNajmabadi What is the observable impact of e55e787? There are disadvantages to both the before and after code and understanding the impact will help with the decision:

  • Before code: disadvantage of changing the behavior of the algorithm. A few more nodes will be visited than before.
  • After code: disadvantage of pushing the either tree of nodes into a stack, which is likely to frequently be very large (possibly prohibitively so)

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 13, 2019 •
edited
Loading

@CyrusNajmabadi What is the observable impact of e55e787? There are disadvantages to both the before and after code and understanding the impact will help with the decision:

It was actually incorrect. i.e. it didn't highlight things properly. this was because it dove into nodes it shoudln't (i.e. an async lambda inside an async lambda).

After code: disadvantage of pushing the either tree of nodes into a stack, which is likely to frequently be very large (possibly prohibitively so)

Note: thsi was the same in both cases. calling DescendantNodesAndSelf works internally using an explicit (pooled) stack. You always pay this cost.

@sharwell
Copy link
Contributor

sharwell commented Dec 13, 2019

Note: thsi was the same in both cases. calling DescendantNodesAndSelf works internally using an explicit (pooled) stack. You always pay this cost.

The part I missed was the use of yield return between pushing things to the stack. I was looking at it as pushing all nodes in the tree to the stack before starting to visit the children. Thanks for the clarification.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 13, 2019

Thanks for the clarification.

Absolutely!

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 13, 2019

@sharwell good to merge this in?

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Dec 13, 2019
Merged
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 13, 2019

@sharwell everything has passed. merge when you can, thanks!

@sharwell sharwell merged commit 0c056f0 into dotnet:master Dec 13, 2019
@CyrusNajmabadi CyrusNajmabadi deleted the asyncAwaitIterative branch December 13, 2019 22:49
@sharwell sharwell added this to the 16.5.P2 milestone Dec 13, 2019
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 13, 2019

Thanks!

@sharwell sharwell mentioned this pull request Dec 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@AdamSpeight2008 AdamSpeight2008 AdamSpeight2008 left review comments

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dpoeschl dpoeschl

@sharwell sharwell

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

16.5.P2

Development

Successfully merging this pull request may close these issues.

VS crash. C# code analysis out of stack due to recursion in AsyncAwaitHighlighter.HighlightRelatedKeywords

5 participants

@CyrusNajmabadi @AdamSpeight2008 @sharwell @dpoeschl @jinujoseph